Repository navigation
Conversation
Exercise the reader Retry callback through the observation lifecycle and real transcript controller. Cover initial recovery failure, repeated Retry, session handoff, observation replacement, and unmount. Generated-by: Codex
Keep the observation controller available before the first transcript publication so Retry can recover after the initial read and automatic recovery fail. Fence retry admission and late errors by session selection and observation identity. Generated-by: Codex
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Full review of 5948cb61 (two commits: d72579a0 adds the test and 5948cb61 the fix; +270/-7, 5 files). This PR hasn't been reviewed before, and it doesn't link an issue.
What changed. The observation effect now records { sessionId, controller } in a new workspace.observationRef, and clears it in cleanup only if it still points at the same observation. retryMessages reloads that controller instead of transcriptRangeRef.current, which is set only after a successful publication. It is guarded by observation.sessionId === sessionId and isSessionSelected(sessionId), and the error report is rechecked after the await.
Correctness.
- The guard passes in exactly the failure case.
isSessionSelectedrequires the published idactiveIdRefto equal the session (session-workspace-actions.ts:231). On a read failure,applyReadErrorcallscommitTranscript(sessionId, [])(use-conversation-observation.ts:71), which setsactiveIdRefand leavestranscriptRangeRefundefined. So after a failure Retry can proceed, while Retry for a different selected Session is still rejected. - No controller divergence. Only the observation effect publishes into
transcriptRangeRef(viapublishTranscript/commitTranscript). There is one controller per effect run: re-subscribing to events reuses it. SoobservationRef.controlleris always the controller that is displayed, or would be once published. - Lifecycle. The identity-checked clear in cleanup handles StrictMode re-mounts and authority-revision replacement. A reload in flight when the effect is torn down is closed by
controller.close(), and its error is suppressed by the post-await identity and selection check.messageRetryPendingis released infinally. refreshMessagesstill usestranscriptRangeRefon purpose, since it needs a published range. That matches the stated scope.
Findings
- P3: a failed Retry shows two error toasts (see the inline comment on
transcript-commands.ts:58). - Nit, no grade:
use-conversation-observation.ts:51uses an inlineimport('../model/conversation-workspace.js')type. A top-levelimport typewould match the rest of the file.
Tests. There are 10 cases that drive the real createDesktopTranscriptRangeController through ConversationLifecycle. They cover:
- Retry after the initial read and the automatic recovery both fail (proved RED before the fix)
- automatic recovery alone, and a failed Retry followed by a successful one
- an A to B handoff, observation-authority replacement, and unmount with a pending read
No test asserts the toast count. Putting the test in apps/desktop/src/main/__tests__ follows the existing convention (47 conversation tests live there).
i18n: no UI copy changes, so all three locales are unaffected. Protocol: not touched.
Status: MERGEABLE (merge state BLOCKED, review required). The head is on current main 3597abe8, git merge-tree is clean, and git diff --check is clean. Hosted CI (label and test) passes on 5948cb61. I did not run the suites or the Electron UI locally. This review is not merge approval.
The range controller's reload() hands a failure to its recovery, which reports it through the observation's onError (the "Failed to load task" toast and read error), and then rethrows. retryMessages reported the rethrown error again as a refresh failure, so one failed Retry raised two toasts and replaced the read error with the refresh message. Leave the report to the controller, as its own gap and recovery reloads do, and only release the pending Retry. The controller still withholds a reload that lands on the cached transcript, so Retry stays quiet then too. refreshMessages never calls reload() and keeps its own report. The retry test now records error toasts and checks that a failed Retry raises exactly one, keeps the read error, and that a successful Retry raises none.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review 5948cb61 → d168c626 (one commit: d168c626d). The merge-base is still 3597abe8 (current main).
Prior findings
- P3, a failed Retry shows two toasts: fixed.
retryMessagesno longer reports.controller.reload()already reports throughrecovery.transcriptFailed→ the observation'sonError→applyReadError.- The old stale guard (observation still current and Session selected) has an equivalent in the observation effect's
!disposedcheck. - The test now wraps
toast.error. It asserts exactly onemessageReadFailedTitletoast with the original read error,messageLoadErrorpreserved, and no toast on the following successful Retry.
- Nit, inline
import()type: withdrawn. Lines 40 and 46 ofuse-conversation-observation.tsuse the same pattern, so line 51 is consistent with the file.
New code
- P3 (low):
transcript-commands.ts:59. While the range holds a cached snapshot, a failed Retry is now silent (see inline).
i18n / protocol: neither is touched.
Status: MERGEABLE (merge state BLOCKED, review required). git merge-tree and git diff --check are clean. Hosted CI test passes on d168c626. I did not run the suites or the Electron UI locally. This review is not merge approval.
The range controller does not report a failed reload while it holds the cached transcript Main serves during Host reconnection. Since Retry left reporting to the controller, a Retry clicked there (for example after a refresh had set a load error) failed with no feedback at all. Expose that state as holdsCachedTranscript() on the range controller and its conversation port, reusing the controller's own cached check. When reload() rejects, retryMessages now reports the refresh error itself only if the controller holds the cached transcript and the observation is still current and selected. Every other failure is still reported once, by the controller's onError. The retry test can now answer a read with a cached snapshot. It shows a failed Retry over that snapshot raising one refresh toast, alongside the existing single load toast for the live case. The other conversation test fakes return false for the new method.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review d168c626 → 99f2df61 (one commit: 99f2df61a). The merge-base is still 3597abe8 (current main).
Prior findings
- P3 (low), a failed Retry is silent over a cached transcript: fixed. The Retry catch now reports only when
holdsCachedTranscript()is true and the observation is still current and selected (transcript-commands.ts:60-62). That is exactly the case where the range store'sonErrorstays silent (!cached(),desktop-transcript-range-store.ts:203). Both read the same flag in the same failure chain, so a failure is reported once, never zero times or twice. The new test covers a failed live read over a cached answer, followed by onetranscriptRefreshTitletoast on the failed Retry. The cached messages and load error are kept. A later successful Retry replaces the transcript and adds no toast.
New code: no issues. The new holdsCachedTranscript port is added to all three test stubs.
i18n / protocol: neither is touched.
Status: MERGEABLE (merge state BLOCKED, review required). git merge-tree and git diff --check are clean. Hosted CI test passes on 99f2df61. I did not run the suites locally. This review is not merge approval.
This branch has not been deployed
2 participants
Summary
When the first transcript read and its automatic recovery both fail, Retry does nothing because it can only reach a controller after a successful publication. Keep the current observation controller available to Retry so the next read restores the conversation. Check session selection and observation identity to reject stale commands and errors.
This covers transcript Retry only. Published-history readiness and the observation effect's cleanup ownership remain intact.
Verification
Retry must issue another transcript read before any publication: 2 !== 3. The 10 new regression cases now pass, and 153 related conversation, transcript, and navigation tests pass throughnode --test.npm run build,npm run typecheck,npm run lint,npm run format:check, and Knip for desktop and UI pass. Renderer architecture against the base, Astryx surface inventory, Windows inventory, app-shell hooks, E2E budget, and ASF headers pass.AI use
Tool(s) and scope: Codex with GPT-6 Astra for investigation, implementation, regression tests, review, and verification. Both commits include
Generated-by: Codex.Checklist
Does this PR entail a change in behavior?